Reject RESPECT NULLS and IGNORE NULLS for aggregate functions
#15014
Conversation
Adds validation to prevent using RESPECT NULLS and IGNORE NULLS with aggregate functions
| @@ -5893,15 +5889,11 @@ SELECT FIRST_VALUE(column1 ORDER BY column2) FROM t; | |||
| ---- | |||
| NULL | |||
|
|
|||
| query I | |||
| query error DataFusion error: Error during planning: RESPECT NULLS and IGNORE NULLS are not supported for aggregate functions | |||
| SELECT FIRST_VALUE(column1 ORDER BY column2) RESPECT NULLS FROM t; | |||
There was a problem hiding this comment.
I am not sure about this change -- the query seems valid to me as there is a null and an argument order, right?
There was a problem hiding this comment.
This is a bit of the weirdness I pointed out in #15006, which is that DataFusion has first_value as both an aggregate and window function, whereas most other engines only have a window function form.
This invocation is technically the aggregate function form, because there is no OVER clause which would be expected for a window function invocation.
There was a problem hiding this comment.
I agree -- I think spark behaves this way which is why someone added the feature to DataFusion
There was a problem hiding this comment.
Ha, there's always another engine 😅
It looks like Spark defines afirst_value(expr[, isIgnoreNull]) function which takes an optional boolean parameter to control whether it ignore nulls.
Spark Docs: https://spark.apache.org/docs/latest/api/sql/index.html#first_value
| @@ -349,6 +349,12 @@ impl<S: ContextProvider> SqlToRel<'_, S> { | |||
| } else { | |||
| // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function | |||
| if let Some(fm) = self.context_provider.get_aggregate_meta(&name) { | |||
| // Reject RESPECT NULLS and IGNORE NULLS for aggregate functions | |||
| // See https://github.com/apache/datafusion/issues/15006 | |||
| if null_treatment.is_some() { | |||
There was a problem hiding this comment.
I would consider it to be a parser responsibility, for example duckDB
D SELECT LAST_VALUE(column1) RESPECT NULLS FROM t
;
Parser Error: syntax error at or near "RESPECT"
LINE 1: SELECT LAST_VALUE(column1) RESPECT NULLS FROM t
^
There was a problem hiding this comment.
Yes I totally agree! Does this mean I should make change in https://github.com/apache/datafusion-sqlparser-rs ?
There was a problem hiding this comment.
I think someone needs to go see what spark does too -- as I recall this was some feature from spark that someone added explicitly...
Maybe @andygrove or @huaxingao remembers 🤔
Which issue does this PR close?
Rationale for this change
As per the issue says
What changes are included in this PR?
Adds validation to prevent using
RESPECT NULLSandIGNORE NULLSwith aggregate functionsOne thing to note that is:
Accroding to the sqllogic test, this should only affect aggregate function.
Are these changes tested?
Essisting tests have coverd the cases
Are there any user-facing changes?
Maybe? If one uses
RESPECT NULLSandIGNORE NULLSwith aggregate functions then this is a breaking change